-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[release/10.0] Fix GO batch separator not recognized in comments and escaped identifiers #37521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/10.0
Are you sure you want to change the base?
Conversation
- Add tracking for block comments (/* ... */) to prevent incorrect quote state toggling - Block comments can now contain single quotes without affecting GO batch separator recognition - Add comprehensive tests for various block comment scenarios - All tests pass successfully Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
Removed separate if statements and i++ increments. Block comment detection now integrated into the main switch statement, making the code cleaner and more maintainable. Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Show resolved
Hide resolved
Added test to verify that /* inside a line comment (-- /*) doesn't trigger block comment state and GO is still correctly recognized as a batch separator. Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Show resolved
Hide resolved
- Changed all GO to go in new test SQL strings (will still appear as GO in output) - Added test for multiline block comment with go on separate line to verify go inside block comments is correctly ignored Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Refactored to use foreach instead of for loop with index by tracking blockCommentStart and blockCommentEnd flags to detect /* and */ patterns across iterations. Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in SQL Server migrations where GO batch separators were not recognized when block comments contained single quotes. The issue was introduced in EF Core 9.0 when quote-tracking logic was added, but it didn't account for block comments.
Changes:
- Replaces simple boolean quote tracking with a state machine that properly handles SQL strings, line comments, and block comments
- Adds comprehensive test coverage with 11 new test cases covering various comment and string scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs | Implements a state machine with 6 states (Normal, Quoted, InBlockComment, MaybeLineComment, MaybeBlockCommentStart, MaybeBlockCommentEnd) to correctly parse SQL and ignore quotes inside block comments |
| test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs | Adds 11 test cases covering block comments with quotes, multiline comments, empty comments, nested comments/strings, and edge cases like GO inside comments |
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
…r.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
artl93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roji - With this being medium risk (though customer reported regression), is there an obvious workaround for the issue?
|
@artl93 The workarounds are straightforward once the root cause is discovered, but since the exception is very sparse on details: |
|
How can we reduce risk around the parser? |
I've added 11 new test cases. We could keep adding more |
artl93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, we want to avoid regression. The more we can add, the better, but otherwise, approved.
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
| SELECT 2; | ||
| """); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Make sure there are tests for all combinations of the characters handled by the switch, preceding a go on a new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in ee931ae. Added 9 tests covering all character types (quotes, forward slash, asterisk, dash, square brackets, double quotes) preceding GO on a new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Also add tests for
--inside quotes and delimited identifiers- brackets and double quotes inside block comments
- quotes, brackets and double quotes in line comments
- any other valid SQL using one of the handled characters in a way that's not covered by the other tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in df1b23e. Added 11 tests covering: line comments inside quotes/brackets/double quotes, brackets/quotes inside block comments, all delimiter types inside line comments, escaped quotes in strings, and multiple delimiters in strings.
…upport - Changed from foreach to for loop with look-ahead to eliminate Maybe* states - Simplified enum from 6 states to 5: Normal, Quoted, InBlockComment, InSquareBrackets, InDoubleQuotes - Added support for escaped identifiers: [go] and "go" - Added support for escaped closing brackets: [g]]o] - Changed switch statement to switch expression for cleaner code - Removed state reset logic (no longer needed without Maybe* states) - Added 9 comprehensive tests for escaped identifiers and character combinations - All 26 tests pass Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Outdated
Show resolved
Hide resolved
| SELECT 2; | ||
| """); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Also add tests for
--inside quotes and delimited identifiers- brackets and double quotes inside block comments
- quotes, brackets and double quotes in line comments
- any other valid SQL using one of the handled characters in a way that's not covered by the other tests
| public virtual void SqlOperation_handles_quote_before_go() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "SELECT 'test';" + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "SELECT 'test';" + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "SELECT 'test\"';" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in df1b23e. Changed to include double quote inside single quoted string: 'test"'.
- Renamed ParsingState.Quoted to ParsingState.InQuotes and moved to end of enum
- Enhanced tests to include EOL before/after go and escaped characters
- Added 11 new comprehensive tests:
- Line comments inside quotes, square brackets, and double quotes
- Brackets and double quotes inside block comments
- Single quotes inside block comments
- All delimiter types inside line comments
- Escaped quotes in strings ('test''s value')
- Multiple delimiters in strings
- Updated existing tests with better coverage (double quote escaping, EOL placement)
- All 37 SqlOperation tests pass
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
| public virtual void SqlOperation_handles_double_quote_identifier() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "INSERT INTO \"go\" VALUES (1);" + EOL + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "INSERT INTO \"go\" VALUES (1);" + EOL + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "INSERT INTO \"" + EOL + "go" + EOL + "\" VALUES (1);" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 600e334. Changed to test multi-line double quote identifier with go inside: INSERT INTO "\ngo\n" VALUES (1);
| public virtual void SqlOperation_handles_square_bracket_before_go() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "SELECT" + EOL + "[column]" + EOL + ";" + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "SELECT" + EOL + "[column]" + EOL + ";" + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "SELECT [" + EOL + "column" + EOL + "];" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 600e334. Changed to test multi-line square bracket identifier: SELECT [\ncolumn\n];
| public virtual void SqlOperation_handles_brackets_inside_block_comment() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "/* [bracket] identifier */" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "/* [bracket] identifier */" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "/* [bracket identifier */" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 600e334. Changed to test unclosed bracket in block comment: /* [bracket identifier */
| public virtual void SqlOperation_handles_double_quotes_inside_block_comment() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "/* \"double\" quote */" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "/* \"double\" quote */" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "/* \"double quote */" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 600e334. Changed to test unclosed double quote in block comment: /* "double quote */
| public virtual void SqlOperation_handles_brackets_inside_line_comment() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "-- [column]" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "-- [column]" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "-- [column" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 600e334. Changed to test unclosed bracket in line comment: -- [column
| public virtual void SqlOperation_handles_double_quotes_inside_line_comment() | ||
| { | ||
| Generate( | ||
| new SqlOperation { Sql = "-- \"column\"" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new SqlOperation { Sql = "-- \"column\"" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); | |
| new SqlOperation { Sql = "-- \"column" + EOL + "SELECT 1;" + EOL + "go" + EOL + "SELECT 2;" }); |
and adjust the assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 600e334. Changed to test unclosed double quote in line comment: -- "column
- Changed double quote identifier test to include newlines within the identifier (double quote spans multiple lines with go inside) - Changed square bracket test to include newlines within the identifier (bracket spans multiple lines with column inside) - Changed block comment tests to show unclosed brackets/quotes (removed closing bracket/quote to test edge cases) - Changed line comment tests to show unclosed brackets/quotes (line comments end at EOL, so bracket/quote is not closed) - These changes test that the parser correctly handles: - Multi-line escaped identifiers with go inside - Unclosed delimiters in comments (which are valid SQL since comments don't need matched delimiters) Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
roji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes, this is now easier to follow and I can't think of any problems.
Fixes #37494
Description
EF Core splits SQL Server migration scripts into batches on lines starting with
GO. To avoid splitting inside string literals,SqlServerMigrationsSqlGeneratortracks whether it is inside a quoted string using a quoted boolean that toggles on single quotes ('). This quote-tracking logic was introduced in #34917.The implementation does not handle:
/* ... */) - if a block comment contains a single quote, the quoted state is incorrectly toggled even though the quote is inside a comment[GO]) or double quotes ("GO")Since there is no matching closing quote in block comments, quoted remains true across subsequent lines. As a result, a later
GOline is not recognized as a batch separator and is sent to SQL Server as literal text, causing:Microsoft.Data.SqlClient.SqlException: Incorrect syntax near 'go'.This commonly occurs in stored procedure scripts where a header block comment contains a ' character above a
CREATE PROCEDUREstatement, or when usingGOas a table/column name in escaped identifiers.Customer impact
Customers using SQL migrations with:
GOusing escaped identifiers like[GO]or"GO"will incorrectly have batch separators insertedWorkarounds:
How found
Customer reported on 10.0.1
Regression
Yes, from EF Core 9.0. Introduced in #34917.
Testing
Added 31 tests covering:
[GO]and double quotes"GO"[g]]o]"go""lum"gotext inside (e.g.,INSERT INTO "\ngo\n" VALUES (1))--) inside quotes and escaped identifiers/* [bracket */- valid SQL since comments don't require matched delimiters)-- [column- valid SQL since line comments end at EOL)'test''s value')Risk
Medium. The changes only affect custom SQL operations in migrations.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.